Skip to content

[Console] Fix incorrect example for CommandCompletionTester #17445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

weitzman
Copy link
Contributor

No description provided.

@carsonbot carsonbot added this to the 5.4 milestone Nov 19, 2022
@xabbuh
Copy link
Member

xabbuh commented Nov 19, 2022

I wonder if showing this test does actually serve any purpose then. 🤔

@OskarStark
Copy link
Contributor

Maybe @wouterj can help us out here

@wouterj
Copy link
Member

wouterj commented Nov 19, 2022

Thanks for starting this PR!

What about only updating the comment, to make it clear when this is useful? For example:

// If you filter the values inside your own code (not recommended, unless you
// need to improve performance of e.g. a database query), you can test this
// by passing the user input
$suggestions = $tester->complete(['Fa']);
$this->assertSame(['Fabien', 'Fabrice'], $suggestions);

@OskarStark OskarStark changed the title Fix incorrect example for CommandCompletionTester Fix incorrect example for CommandCompletionTester Nov 20, 2022
@carsonbot carsonbot changed the title Fix incorrect example for CommandCompletionTester [Console] Fix incorrect example for CommandCompletionTester Feb 21, 2023
@javiereguiluz javiereguiluz merged commit 900b38f into symfony:5.4 Feb 21, 2023
@javiereguiluz
Copy link
Member

@weitzman thanks for this contribution ... however, while merging I reworded it as indicated by @wouterj because I think the intention of this feature is more clear that way. Thanks!

@weitzman weitzman deleted the patch-1 branch February 21, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants